codereview: should the Go project stop importing GitHub PRs? #61182
Replies: 17 comments 32 replies
-
|
I started contributing to Go a few months ago and used the Github workflow on my first submission. The especially difficult part was getting formatting conventions right on the commit message: sending updates that rendered differently on Github to match the required format for Gerrit took most of the time. In hindsight, learning the Gerrit workflow from the start would have been much simpler. |
Beta Was this translation helpful? Give feedback.
-
|
I agree with this, and having been forced to do Gerrit extensively (professionally), I also wished I went with Gerrit from the get go. If simple reviews of drive-by contributions could be handled GitHub-side, that would be great. |
Beta Was this translation helpful? Give feedback.
-
|
From my experience contributing to projects hosted on external services is usually cumbersome because it requires extra account registration and small drive by contributions like link fixes are probably not being done anymore. |
Beta Was this translation helpful? Give feedback.
-
|
@mooijtech, “using GitHub for everything” was considered (and declined) in #21956. |
Beta Was this translation helpful? Give feedback.
-
|
@bcmills that proposal had a lot of upvotes. Why was it declined? This is the opposite of that. |
Beta Was this translation helpful? Give feedback.
-
|
@matys1, you can read the discussion on that proposal for the details. 😅 |
Beta Was this translation helpful? Give feedback.
-
For which you need to have a google account, so for the sake of simplicity I am using my private account for work contributions otherwise I would probably still be in the process to get an account to fix a link. Also that the mailing list requires a google account is also a big bummer. I am currently relaying the mails from my private google account otherwise I couldn't receive the security update announcements. |
Beta Was this translation helpful? Give feedback.
-
FWIW, there are people who feel the same about GitHub. That is, there are people who repeatedly complain about not being able to participate in discussions without a GitHub account. |
Beta Was this translation helpful? Give feedback.
-
|
One data point from another project: we also use Gerrit in the CUE project, and we also allow GitHub PRs for convenience, but in a slightly different way to what Go does currently. We do the code review and CI on the GitHub side, and manually import to Gerrit only when a PR is approved and ready to be merged. This approach is still far from perfect, but we think it works a bit better for PR contributors, since their workflow remains pretty natural. The only difference is that we can't and won't merge the PR directly, since the GitHub repository is simply a mirror from Gerrit. On our side it's a little bit more work, since we effectively review code on two platforms, even if Gerrit is the primary one. However, most of us are already used to doing reviews on GitHub, even with its shortcomings. And one could argue that we save work by not having to maintain any form of server or bot to make this all happen; we simply have an It's worth noting that using Gerrit directly is still best in most cases, particularly when someone wants to contribute a big or complex change, or when someone wants to contribute regularly. For those cases, we still kindly ask the person to learn the Gerrit route, and I think that's fine. |
Beta Was this translation helpful? Give feedback.
-
|
I should clarify that we don't guarantee to be able to accept any change as a PR. For example, if a PR is opened with changes to the core evaluator, whose maintainer is Marcel, we'll likely have to ask them to reopen on Gerrit instead since he can't really do code reviews on GitHub. At least from the experience we've had in the past couple of years, most of the PRs we do get we are able to review on GitHub, since they tend to be on the small side and most of the reviewers are capable enough with the platform. |
Beta Was this translation helpful? Give feedback.
-
|
Having slept on it, I think CUE's approach wouldn't work for Go given how the existing experienced reviewers are not used to GitHub at all, and given how the project needs more maintainers and reviewers than it needs new contributors and drive-by patches. I also have no love for the mirror bot for the reasons that Bryan outlined, to be clear. As a middle ground, it often makes neither kind of user (GitHub or Gerrit) happy. That said, the project should realize that such a tradeoff (prioritizing existing maintainers over new contributors by only using Gerrit) is not a great situation for the long-term health of the project. As much as Gerrit is in many ways a much nicer and more powerful review tool, the truth is that most developers today are already familiar with GitHub and prefer its native review process. Assuming that we want to lower the barrier for new contributors to come in and eventually become maintainers, this proposal would discourage that. Clearly it's less work for the existing maintainers and reviewers to only worry about Gerrit, but it's also less likely that that set of people will grow with the project over time if we insist on a tool that is a barrier for some contributors. Ideally, we would "just" make Gerrit more approachable to new users, but for that I think you might need a UI/UX rethink, or at least some kind of interactive tutorial to learn all the new concepts and web UI buttons :) |
Beta Was this translation helpful? Give feedback.
-
|
For long term health, I think the focus on new contributors feels off. Rather, the process for someone to go from writing to reviewing code is unclear, in terms of process, etiquette, and benefits/responsibilities. |
Beta Was this translation helpful? Give feedback.
-
@rsc Can you list those reasons for the benefit of others reading the thread? Is it because they're at Google and have to use Gerrit anyway? Or because Gerrit is better at handling "stacked changed" (changes that build on other, unmerged changes)? Or something else? In any case, I'm an occasional contributor -- I've only used the Gerrit method, because I'm allergic to two-way syncs. Though learning the Gerrit method was slightly annoying, |
Beta Was this translation helpful? Give feedback.
-
|
A few advantages that come to mind: Gerrit is good at supporting line-by-line code reviews, and tracks for each comment whether it has been addressed or not (our auto-submit bot won't submit a change with unresolved comments). Gerrit maintains an "attention set" for who should look at the change next. Gerrit provides dashboards of changes that need review by different people. It's also true, as you say, that Gerrit does a good job of handling stacked changes. We also have a lot of machinery built around Gerrit, like the builders and auto-submit. |
Beta Was this translation helpful? Give feedback.
-
|
I contributed my first (and currently only) change to the Go project through GitHub a few years back when I was a relatively fresh developer. There were definitely a few quirks with the workflow and I remember having various issues getting titles and commit messages updated but it turned out well in the end. In hindsight, I don't think I would have considered contributing if it wasn't already possible though a service like GitHub where I already felt confident in the tooling. Even to this day I feel similarly about various other projects that are not on GitHub or GitLab (especially using mailing lists, phabicator, etc.) because the step up in learning might not be worth it if I don't want (or have the time) to become a regular contributor. That being said, I could consider contributing through Gerrit in the future because I already have some minor knowledge of how it works from last time (and I care about the project enough). Removing the support for importing PRs from GitHub would likely result in less new developers contributing to the project. There do however seem to be a vast majority of problems with how it is currently done and I believe that the best solution would be to improve the PR workflow through GitHub, if possible. If not, I would not personally be against removing the support as the current cons seem to outweigh the pros in many ways. |
Beta Was this translation helpful? Give feedback.
-
|
I think it would be nice to have some measurement of the review friction mentioned above; "a large fraction of code reviews for GitHub PRs require multiple rounds of comments of the form “please mark comments as resolved in Gerrit if they have been addressed”", etc. I started such an analysis tool in https://go.dev/cl/507907, which could be extended to look at how many back-and-forth comments GitHub pull requests have. As I put this together quickly, I only looked at the proportion of merged CLs, which is indeed lower with GitHub, even for "new" contributors: (This tool suspiciously "finished" fetching at exactly 10000 CLs, so I think I hit a Gerrit limit rather than actually analyzing all CLs). |
Beta Was this translation helpful? Give feedback.
-
|
On the other hand, this data shows that nearly 50% of CLs from "new" contributors are coming from GitHub, and ~33% of merged "new" contributor CLs. |
Beta Was this translation helpful? Give feedback.
-
|
Yep, but that also doesn't tell us what would have happened without GitHub import — we unfortunately can't measure what fraction of those contributor CLs would have been sent directly through Gerrit vs. not sent at all. |
Beta Was this translation helpful? Give feedback.
-
|
We can see how many CLs were contributed before the Gerritbot was added compared to after. Of course the community grows over time, but perhaps we can adjust for that. |
Beta Was this translation helpful? Give feedback.
-
|
In another thread, @bcmills referenced #21956 "Just use github for everything" as declined. The issue however, was closed with
Not that it was declined. Just that it was off-track. (this was before the proposal processes) Regarding that proposal, I think the amount of discussion that it received, and the number of people who 👍 it means that a large portion of people would prefer using GitHub. I suspect that still rings true today. A key thing that was supposed to happen from that issue was the documentation of the key differences (#22002) between gerrit and github and what the community wanted from its contributing/reviewing experience. That was ultimately closed due to the creation of the very bot of this discussion. I think #22002 should be re-opened/recreated and done first, and only then a more general decision had regarding how to improve PRs and contributions in general (where using Github vs Gerrit vs both could be discussed). Should the discussion there decide that gerrit is the only solution, the bot can be removed. As it stands, I believe this discussion is suggesting a solution to a premature question. |
Beta Was this translation helpful? Give feedback.
-
|
Note that #22002 was filed for the folks arguing for switching entirely to GitHub to assemble a plausible proposal for actually doing so. It was open and labeled If you would like to pick up that work, you are welcome to collect the requested information and present a new proposal as described in that issue. However, I would expect that sort of migration to be much more labor-intensive than what is under consideration in this discussion — I don't agree that considering smaller-effort improvements to the status quo is “premature”. |
Beta Was this translation helpful? Give feedback.
-
|
I strongly agree with using exactly one review tool. It's unfortunate for the project if the issue tracker is on Github and reviews are on another site, because linking between the two is inconvenient, labeling is harder to keep consistent. But I think the project has a few problems with that with the bidirectional synchronization already. As a (rare) contributor, I think that the review comments I get via Gerrit are significantly better than in Github. They can be ordered as the reviewer desires, whereas Github comments are sorted by the order that they're added to the draft. Understanding the scope of the requested changes can be difficult in Github. |
Beta Was this translation helpful? Give feedback.
-
|
Whilst the vast majority of code on the Go ecosystem exists primarily on Github, it seems like the maintainers have a preference for the workflow of Gerrit. Instead of suggesting changes to the Gerritbot, workflow engine, or source of truth, would it not make more sense to poll for familiarity of the both tools, and make proposals based on those going forward? We take a similar approach to a lot of other changes to the language and ecosystem. I do not see why we would risk making the language “less approachable for new contributors”, “more difficult to manage for those already familiar with Gerrit”, or “require specific knowledge of GitHub/Gerrit/other tool” without data to baseline the conversation. |
Beta Was this translation helpful? Give feedback.
-
|
Poll whom? Existing contributors are necessarily going to be at least somewhat familiar with Gerrit, and there is no reliable way to identify people who have not yet contributed but would do so in the future. |
Beta Was this translation helpful? Give feedback.
-
|
Push commit to github pr is still more convenient than to solve the data model mismatch problem, I think there is another solution. Not to stop importing github PR totally, but stop 2 way sycning, and limit the usage of github PR. Bot will still import code from github PR as replacement of And don't sync code review back to githuh PR, force contributors to use Gerrit for code review and discussion, only let bot add a comment with its gerrit link, and maybe, lock conversation of PR or just omit them. github PR support check, so a bot can detect misused github features, for example, markdown description or not gofmt-ed source code, show a error in github PR check, and don't import them. In this way, contributors are familiar with github but not Gerrit will only need to learn how to use gerrit web, which imo is easy. No need to learn go codereview cli, which imo is very hard to use. |
Beta Was this translation helpful? Give feedback.
-
|
Yes. I'm an occasional contributor. I made one or two contributions before the Github bridge, but since then I've been doing it by opening a PR. I find it annoying to have to wait for the bot to sync commits to Gerrit and bounce back and forth, but overall, it's easier for me to open a Github PR than to figure out how to use Gerrit directly (since I'm in Github all day anyway), so I use the Github bridge. It probably wouldn't affect how much I contribute if the bridge went away, but as things stand now I prefer to use it. |
Beta Was this translation helpful? Give feedback.
-
I don't see how this would reduce the need to learn the Gerrit git-codereview CLI. Wouldn't they still need to use it to push edits when they address comments anyway? (#61182 (comment) is a similar thread.) |
Beta Was this translation helpful? Give feedback.
-
|
|
Beta Was this translation helpful? Give feedback.
-
|
Not sure I understand this thread. My contributions to Go have required back and forth, and I never used the git-codereview CLI. I push additional commits to the PR branch and they're imported into Gerrit. Example #59489. Comments have mostly happened on Gerrit web, not PRs. |
Beta Was this translation helpful? Give feedback.
-
|
I think the right thing to do would be to change the bot which mirrors GitHub PRs to gerrit, to instead immediately close the GitHub pr and share a link to a replacement on gerrit, with an explanation of what happened. Fully removing support for GitHub prs is going to dissuade a lot of people from ever even trying to become a contributor, but if you just accept initial commits via GitHub and then force people to gerrit I think it is much less harsh, and i think gets you most of the centralized workflow wins you are interested in. I hope you dont fully shut down github PRs. |
Beta Was this translation helpful? Give feedback.
-
|
That's an interesting idea — it would address many of the problems around the data model mismatch. I still worry that it would lead to a lot CLs abandoned or with unclear status, though. If someone sends the change through Gerrit in the first place, we know they've successfully signed in to Gerrit and they are set up to push edits to it too. |
Beta Was this translation helpful? Give feedback.
-
|
I like this idea, but I'm not certain it is possible to set up permissions to allow the author to push a new patch set to GerritBot's version of the CL (especially if the commit "author" is GitHub's noreply email rather than the actual author). A similar option would be to auto-close PRs with a copy-pastable CLI snippet to automatically upload the same PR to Gerrit (add remote, add change-id hook, amend commit, push). |
Beta Was this translation helpful? Give feedback.
-
|
Don't get me wrong but for a big project (like Go) Gerrit is a good firewall which raises a bar and prevents tons of PRs with a questionable changes. As a result it keeps core team less disrupted. (sorry, don't have any numbers, just gut feelings) |
Beta Was this translation helpful? Give feedback.
-
|
Manishearth discussed this in the original "switch to GitHub" thread: #21956 (comment) |
Beta Was this translation helpful? Give feedback.
-
|
The GitHub |
Beta Was this translation helpful? Give feedback.
-
|
We have tooling, written in Go, to manage submitting changes to Gerrit. See the mentions of |
Beta Was this translation helpful? Give feedback.
-
|
This answer may not be suitable for reply here, but there isn't a more appropriate place found. The choice of a tool for code review should not only be considered from the perspective of code review. I believe we face yet another issue. The inability of Google search engine to index the content from Gerrit poses a problem. This implies that one cannot access the content they are interested in through Your only choice is to perform an internal site search: What is my current strategy when I need to search for relevant issues?
What are the limitations of this approach?I am unable to find comments made during the review process. What's my ideal scenario?I should be able to find what I am looking for through Google/GitHub/Gerrit or any other platform. |
Beta Was this translation helpful? Give feedback.
-
|
This seems a bit off-topic. I think there is a Send Feedback option in the Gerrit UI that you can use to report it to the appropriate maintainers. |
Beta Was this translation helpful? Give feedback.
-
|
We should consider staying on GitHub and taking inspiration from the Flutter project's success in building a vibrant community. |
Beta Was this translation helpful? Give feedback.
-
Hi @jnelle, just in case there was some ambiguity from some of the other comments, this discussion is not about leaving GitHub, and to my knowledge, no one is proposing that. This discussion is just about the much narrower question of what to do about inbound GitHub PRs. From the Flutter contribution guide, it looks like the Flutter project uses GitHub PRs for reviews? FWIW, I think it is unlikely that the Go project would drop Gerrit in the near term and rely entirely on GitHub PRs for reviews. See for example Ian's comment here. |
Beta Was this translation helpful? Give feedback.
-
|
See #45246 If you stop importing GitHub PRs, how can you solve the following situation: remote: PERMISSION_DENIED: The caller does not have permission remote: [type.googleapis.com/google.rpc.LocalizedMessage] remote: locale: "en-US" remote: message: "go.googlesource.com is not available from your location." remote: remote: [type.googleapis.com/google.rpc.RequestInfo] remote: request_id: "c922a8c106674b4f9de1731653555818" fatal: unable to access 'https://go.googlesource.com/go/': The requested URL returned error: 403 (running: git push -q origin HEAD:refs/for/master) git-codereview: exit status 128 |
Beta Was this translation helpful? Give feedback.
-
|
@qiulaidongfeng, I think I must be missing something. If you can't access https://go.googlesource.com/go in order to push to it, how do you access https://go-review.googlesource.com in order to address comments on the code review? If |
Beta Was this translation helpful? Give feedback.
-
|
I modified "C:\Windows\System32\drivers\etc\hosts" this file access https://go-review.googlesource.com and https://go.googlesource.com/go . This is the ping result. PING go-review.googlesource.com (2607:f8b0:4024:c02::52): 32 bytes from 2607:f8b0:4024:c02::52: icmp_seq=0 time=244.0446ms ttl=-1 32 bytes from 2607:f8b0:4024:c02::52: icmp_seq=1 time=246.9382ms ttl=-1 32 bytes from 2607:f8b0:4024:c02::52: icmp_seq=2 time=250.1194ms ttl=-1 32 bytes from 2607:f8b0:4024:c02::52: icmp_seq=3 time=247.7303ms ttl=-1 PING go.googlesource.com (2607:f8b0:4004:c17::52): 32 bytes from 2607:f8b0:4004:c17::52: icmp_seq=0 time=261.9278ms ttl=-1 32 bytes from 2607:f8b0:4004:c17::52: icmp_seq=1 time=257.764ms ttl=-1 32 bytes from 2607:f8b0:4004:c17::52: icmp_seq=2 time=252.8123ms ttl=-1 32 bytes from 2607:f8b0:4004:c17::52: icmp_seq=3 time=259.2603ms ttl=-1 At present, except for the git codereview email, all other parts of the contribution guide are okay. Through #45246, this #20065 (comment) seems to explain why. |
Beta Was this translation helpful? Give feedback.
-
|
As somebody who occasionally contributes to Go I do like GitHub interaction. I have not found many (any?) issues with it. I make a PR like I am used to, I get a link to the Gerrit by the bot, I go there, and I star it with the account I have signed in there using my Gmail account. If needed, I respond to comments in Gerrit. But otherwise I just push new commits to my branch and PR is updated, and Gerrit as well. So I have found this very useful in the sense that I do not really need to learn how to use Gerrit in terminal, I use it just for comments through the browser. The rest I just do regular PRs. About commit message formatting, I think that would be the issue with using just Gerrit as well. Go requires the message to be formatted in a particular way so one has to learn that. One middleground solution could be that some basic git commit message checks are added to GitHub Actions and Gerrit bot is run only after that is cleared. |
Beta Was this translation helpful? Give feedback.
-
|
I discussed this with a broader subset of the Go team. @thepudds (who is not on the Go team, but is a valued contributor!) has been making significant improvements for many of the friction points I named at the start of the discussion. The consensus from internal discussions is that we would like to see how those improvements pan out, and if they end up reducing the friction of GitHub PRs by as much as we expect, then it is worthwhile to continue importing GitHub PRs. Thanks everyone for the feedback and ideas, and especially to @thepudds for putting those ideas into action. |
Beta Was this translation helpful? Give feedback.
and others
-
In #18517 (accepted Jan. 2017), we decided to start importing GitHub pull requests for the Go repositories as Gerrit changes, in addition to our more typical direct-to-Gerrit workflow.
In the time since then, I have reviewed many contributed changes to the various Go repos, including those posted as GitHub PRs and those posted directly to Gerrit.
In my experience, the GitHub workflow is not working well.
git-codereviewhooks do not apply for GitHub PRs, so simple reminder steps like “make sure your code isgofmt'd” often require extra rounds of review comments.In addition, the bot that implements the import workflow is somewhat buggy and not always reliable
(see https://github.com/golang/go/issues?q=is%3Aissue+is%3Aopen+gerritbot+in%3Atitle for the complete list).
It may be possible to fix many of the bugs in the GerritBot implementation, at some indeterminate cost. However, I do not believe that it is possible to address the mismatches in the data model in a satisfactory way — especially not without significant additional development work on GerritBot, which I don't foresee becoming a priority for the Go project in the near future.
In this discussion, I would like to investigate the possibility of no longer accepting GitHub PRs, and reducing GerritBot to simply closing them with a message directing the requester to the Gerrit workflow in the Contribution Guide. That would essentially reverse #18517 in light of several years' experience with the result of that proposal.
[edited 2023-08-11] Since this discussion began, a number of these friction points have been addressed. We will continue to import GitHub PRs and see if the friction in that process has been sufficiently reduced. (#61182 (comment))
Beta Was this translation helpful? Give feedback.
All reactions